-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for labels #417
base: master
Are you sure you want to change the base?
Support for labels #417
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just need to add some tests and update the README.md
before merging.
Thank you for your contribution!
Sounds great! I've made the changes locally per your suggestions. I'll add some tests and update the README within the week. |
- Adding support for labels - Mixing in process.env for Azure loadFromAzure
I added a test to ensure |
We could use the toBeCalledWith function to assert that we are passing the expect(appConfigListMock).toBeCalledWith(
expect.objectContaining({
labelFilter: AZURE_APP_CONFIG_LABELS,
})
) |
Another good feature to have is allowing the label filter to be defined programmatically, just like the dotenv-azure/src/dotenv-azure.ts Line 208 in 0ca5e6c
Then we could use it like this: await dotenvAzure.config({ labels: 'production,development' }) or await dotenvAzure.config({ labels: ['production', 'development'] }) |
The CI is failing because of prettier, just run the script below, commit the changes and it should be fixed: npm run format |
I added support for labels which by itself was a pretty simple change by passing the
labelFilter
property intolistConfigurationSettings
. When running this in production for our system, however, the Azure credentials were not being picked up from our container's environment. I realized this was because the credentials were not read fromprocess.env
when loading inconfig()
. I added support for this which required some typing change since it was no longer the same shape as DotenvParseOutput.I'm not married to any of this so open to any suggestions. Thanks for creating this project!